Use NonZeroU32 instead of u32 within Symbol.
#56662
Conversation
This shrinks `Option<Symbol>` from 8 bytes to 4 bytes, which shrinks `Token` from 24 bytes to 16 bytes. This reduces instruction counts by up to 1% across a range of benchmarks. The commit introduces a new type, `SymbolVec`, to encapsulate the 1-indexing now required to inter-operate with the non-zero indices.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Here is the first part of the Cachegrind diff on one benchmark. It shows lots of little improvements all over the parser, as you'd expect. |
|
@bors r+ Seems like a good optimization. |
|
📌 Commit cf682e0 has been approved by |
bors
added
S-waiting-on-bors
|
NonZeroU32 allows to denote one value (zero) as special, so with compiler help Option<> uses it to denote a missing value. But then you have had to implement the 1-indexing. I guess the CPU doesn't pay much those 1-subtractions caused by SymbolVec::get, but an alternative solution is to use a different value, like u32::MAX to denote the missing value for Option<>. This still restricts the u32 range by 1, but doesn't require the subtractions and the 1-indexing code. In the D standard library ( https://dlang.org/phobos/std_typecons.html#Nullable ) there is a second version of Nullable that allows that: |
| /// Symbols (which are 1-indexed) index into this (which is 0-indexed | ||
| /// internally). The methods handle the index conversions. | ||
| #[derive(Default)] | ||
| pub struct SymbolVec(Vec<&'static str>); |
There was a problem hiding this comment.
We could also make Symbol a newtype_index, which would prevent such index arithmetic hacks and even make Option<Option<Symbol>> and deeper nestings take up the same amount of bytes.
That would require no changes to the list of symbols, too
There was a problem hiding this comment.
How does newtype_index avoid the space for the tag when used within an Option?
There was a problem hiding this comment.
It reserves (u32::max_value() - 255)..=u32::max_value() via compiler-internal trickery:
|
@bors r- we should discuss the way this optimization is implemented |
bors
added
S-waiting-on-author
|
I tried this previously (#28657) and found the difference to be lost in noise, but I didn't have proper benchmarks. |
|
If the goal is to shrink |
|
Reserving a slot in the end of integer range instead of 0 (#56662 (comment)) seems like the best solution. |
|
r? @oli-obk |
I wrote a patch doing that a while ago. It makes the code much uglier, and spreads complexity to every piece of code that touches |
|
I opened a new PR (#56699) for the |
This shrinks
Option<Symbol>from 8 bytes to 4 bytes, which shrinksTokenfrom 24 bytes to 16 bytes. This reduces instruction counts by upto 1% across a range of benchmarks.
The commit introduces a new type,
SymbolVec, to encapsulate the1-indexing now required to inter-operate with the non-zero indices.